-
Notifications
You must be signed in to change notification settings - Fork 14k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add https support for Druid #4480
Conversation
96b1260
to
f08d19f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a comment on making the code more robust.
superset/connectors/druid/models.py
Outdated
@@ -107,24 +107,29 @@ def data(self): | |||
'backend': 'druid', | |||
} | |||
|
|||
@staticmethod | |||
def get_base_url(host, port): | |||
if not host.startswith('http'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unlikely, but this might cause problems if the hostname starts with http
, eg, http-server.example.com
. I think it's better to use a regex here:
if not re.match('http(s)?://', host):
host = 'http://' + host
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call
I think |
8e9ef14
to
3fbd230
Compare
Fixed, mergin' |
endpoint = ( | ||
'http://{obj.coordinator_host}:{obj.coordinator_port}/status' | ||
).format(obj=self) | ||
endpoint = self.get_base_coordinator_url() + '/status' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mistercrunch this is broken, it should use get_base_url()
instead.
* Add https support for Druid * addressing comment
* Add https support for Druid * addressing comment
closes #4465